feat(replay): Rework slow click & multi click detection #8322
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/slow-click-rage-click
branch
from
6180df3 to
285d82b
Compare
| } | ||
|
|
||
| // Rage click | ||
| if (clickCount > 3) { |
There was a problem hiding this comment.
I feel we could consider 3 an option. Not from the get go, lets see how this goes.
There was a problem hiding this comment.
yes, makes sense! Maybe we need to/want to guard this behind an experiment(al option) anyhow for now 🤔
There was a problem hiding this comment.
so instead of this arbitrary limit, we now simply capture a new breadcrumb type ui.multiClick with a clickCount, whenever this is > 1.
| // ignore VERY close timestamps - otherwise we record the initial timestamp twice! | ||
| if (click && Math.abs(click.timestamp - nowInSeconds()) > 0.01) { |
There was a problem hiding this comment.
I'm not sure I follow here -- what causes the very close timestamps?
There was a problem hiding this comment.
The problem is that we have two click listeners now. The one from the core breadcrumbs integration triggers the dom breadcrumb ui.click which is recorderd here via handleClick(). Then we have a separate click listener, which triggers right after this. So any click would trigger twice. Made more difficult by the timestamps being slightly off, as the logic we use in the core breadcrumbs integration is a bit more complex, so we can't really do an equal check in there 😬
There was a problem hiding this comment.
Can we separate the code paths so that the click from core breadcrumbs is handled outside and is only responsible for creating the ui.click breadcrumb?
There was a problem hiding this comment.
I've actually changed it up a bit. Completely separating it doesn't work easily as we then don't have access to the rrweb node stuff etc.
BUT I simply start out the clickCount as 0, then I can always increment it - that should work fine actually, I think, as the multi click handler should always run after the other handler.
There was a problem hiding this comment.
ok I'll take another look -- re: rrweb node, we should be able to get that information in the new handler. It'd be duplicating some work, but could be worth it if it simplifies logic.
mydea
force-pushed
the
fn/slow-click-rage-click
branch
from
4e6af85 to
5ad616d
Compare
mydea
changed the title
|
Note: If users disable slow click timeout, we will also not capture multi clicks. So we have this as an escape hatch if somebody runs into a problem. |
|
@mydea are we still going to have this behind an experiments flag? |
This PR reworks the slow click detection to accommodate rage click detection as well.
This required substantial changes, as we need to keep track of stuff much more.
Now, we keep a list of clicks that come in in a new class. We register a single set of listeners (mutation observer, click listener, scroll listener), and then try to route things to the correct clicks as much as possible.
Any clicks within 1 second count as "multi click", so are not considered for slow clicks at all, but counted on the first click. After a second, a click (even on the same element) will be treated as a "new" click.
ref #8300